Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(aliasing): tests import aliasing #977

Conversation

sanketshevkar
Copy link
Member

@sanketshevkar sanketshevkar commented Jan 13, 2025

Closes #972

Changes

Added to new optionalresolvedName property to TypeIdenifier. This is to preserve the aliased name property which will have the local name for the aliased type. resolvedName will contain the origonal type name. It will get added only for the declrations ot types that "imported and aliased".

With the current implementation the value of the name property is different for a normal ast (local name) vs a resolved ast (original type name). This can be very confusing.

The above changes have been released in [email protected].

This PR mostly focuses on adding the unit tests to concerto apis to reinforce the import aliasing feature.

Tests for the following have been added specifically targeting import aliasing:

  • ClassDeclaration
  • Property
  • Decorator
  • Map-Key
  • Map-Value
  • CTO-ResolvedAST-CTO, a test to confirm that the round trip from CTO to CTO is successfull and produces exactly the same model.

Author Checklist

  • Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary
  • Merging to main from fork:branchname

packages/concerto-core/package.json Outdated Show resolved Hide resolved
packages/concerto-core/lib/introspect/declaration.js Outdated Show resolved Hide resolved
packages/concerto-core/lib/introspect/property.js Outdated Show resolved Hide resolved
packages/concerto-core/test/introspect/decorator.js Outdated Show resolved Hide resolved
@sanketshevkar
Copy link
Member Author

What is the expected response in the test? and what is the actual value?

I thought we could extend scalar types bu other scalar types, but that's not the case. I wanted to import alias an scalar type. Declare a new scalar type and extend it with the one which was imported.

@sanketshevkar sanketshevkar marked this pull request as ready for review January 16, 2025 14:17
@@ -33,9 +33,9 @@ function copyFiles(files, destDir) {
}

const lcovs = glob.sync(globPattern).map((dir) => {
const packageName = dir.split('/').pop();
const packageName = dir.split(path.sep).join('/').split('/').pop();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glob had to be updated to fix an issue in rimraf, which was causing concerto-types build to fail.

glob itself had a breaking change for the sync API a few releases back because of which coverge reprting was failing for windows environment in the PR build pipeline.

So I had to fix the script.

}
}
],
"location": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove the locations from the ast please?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is doing a doing a check on the parsed cto model with the ast.


it('should be able get validate a decorator whose argument is an imported type which is aliased', () => {
const classDeclaration = resolvedModelManager.getType('[email protected]');
const decorator = classDeclaration.getDecorators()[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also check the decorator for the whole model?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes works!
Added a new test to validate that.

Signed-off-by: sanketshevkar <[email protected]>
@test(Kid)
namespace [email protected]

import [email protected].{MyString as Str,Child as Kid}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it's jet lag, but I find these examples hard to parse. Let's give the identifiers more descriptive names to make the tests easier to understand.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Signed-off-by: sanketshevkar <[email protected]>
@sanketshevkar sanketshevkar merged commit 41d1ab0 into accordproject:main Jan 17, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

APIs from concerto-core are breaking for aliased models whose types are resolved.
3 participants